BUG: Categorical(Index) passed as categories #17888

jorisvandenbossche · 2017-10-16T08:19:12Z

closes Categorical.from_codes does not allow categories to be a CategoricalIndex #17884
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

@TomAugspurger I am not sure we had a discussion about that before, but to me it does not make many sense to have a CategoricalIndex as the categories of a Categorical/CategoricalIndex, so I think we should convert passed categories (which is what this PR is doing).
Alternative to fix #17884 is to fix the repr code to deal with Categorical categories.

jreback · 2017-10-16T09:21:06Z

pandas/core/dtypes/dtypes.py

@@ -316,7 +315,10 @@ def _validate_categories(categories, fastpath=False):
        from pandas import Index

        if not isinstance(categories, ABCIndexClass):
-            categories = Index(categories)
+            categories = Index(categories, tupleize_cols=False)
+


should be an elif and instead should use is_categorical

Isn't the point of the ABC ones to avoid imports from itself ? (as is_categorical uses CategoricalDtype)

@jreback Can you answer this one? No problem to change it if you think it is better, but it seems to me using isinstance(categories, ABCCategoricalIndex) is consistent with the use of isinstance(categories, ABCIndexClass) just above (also in readability of the code)

hmm, yeah I guess you have to do this. but you can also check isinstance(categories, (ABCCateoricalIndex, ABCCategorical)) is more generic.

updated to use isinstance(categories, (ABCCateoricalIndex, ABCCategorical)) first and then only to check for Index

TomAugspurger · 2017-10-16T12:45:07Z

Thanks Joris, agreed with the changes here. Looks like this conflicts with your fastpath PR.

codecov · 2017-10-16T13:19:39Z

Codecov Report

Merging #17888 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17888      +/-   ##
==========================================
- Coverage   91.23%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50105    50103       -2     
==========================================
- Hits        45714    45704      -10     
- Misses       4391     4399       +8

Flag	Coverage Δ
#multiple	`89.03% <100%> (ø)`	⬆️
#single	`40.31% <83.33%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/dtypes.py	`95.15% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.11%)`	⬇️
pandas/core/indexes/datetimes.py	`95.5% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9092445...ab23d41. Read the comment docs.

codecov · 2017-10-16T13:19:47Z

Codecov Report

Merging #17888 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17888      +/-   ##
==========================================
- Coverage   91.23%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50105    50105              
==========================================
- Hits        45714    45706       -8     
- Misses       4391     4399       +8

Flag	Coverage Δ
#multiple	`89.03% <100%> (ø)`	⬆️
#single	`40.31% <83.33%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/dtypes.py	`95.14% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.5% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9092445...1c56abe. Read the comment docs.

jschendel · 2017-10-16T17:19:50Z

pandas/tests/test_categorical.py

+
+        result = pd.Categorical(
+            ['a', 'b'], categories=pd.CategoricalIndex(['a', 'b', 'c']))
+        tm.assert_categorical_equal(result, expected)


Categorical and CategoricalIndex have been imported, so you can remove the pd. here, if you feel that's cleaner.

yes, thanks! (updated)

jschendel · 2017-10-16T17:20:14Z

pandas/tests/test_categorical.py

+
+        result = pd.Categorical.from_codes(
+            [0, 1], categories=pd.CategoricalIndex(['a', 'b', 'c']))
+        tm.assert_categorical_equal(result, expected)


same regarding pd.

jreback · 2017-10-16T18:01:38Z

doc/source/whatsnew/v0.21.0.txt

@@ -1023,6 +1023,7 @@ Categorical
 - Bug in the categorical constructor with empty values and categories causing the ``.categories`` to be an empty ``Float64Index`` rather than an empty ``Index`` with object dtype (:issue:`17248`)
 - Bug in categorical operations with :ref:`Series.cat <categorical.cat>` not preserving the original Series' name (:issue:`17509`)
 - Bug in :func:`DataFrame.merge` failing for categorical columns with boolean/int data types (:issue:`17187`)
+- Bug in constructing a ``Categorical``/``CategoricalDtype`` when the specified ``categories`` where of categorical type (:issue:`17884`).


I would move this to the CDT issue, IOW the sub-section (as its actually not an independent bug) and better there.

The bug also exists for Categorical and CateforicalIndex and was already present in current released 0.20.3. So not directly related to the CDT rework (although the bug is present in CDT as well). But not that important, can also move if you want.

jorisvandenbossche · 2017-10-16T18:39:08Z

One disadvantage of the my changes is that is somehow relaxes the "unique" requirement for the passed categories. Because now you can pass a non-unique Categorical, and we don't raise anymore but just take its unique categories.
I think it seems obvious that this is what the user want, but still, I am not sure relaxing the requirement is a good idea (eg in from_codes it means the passed codes don't match exactly the passed categories). But actually I can easily check this by moving the check for Categorical values after the check for unique / nulls.

jreback · 2017-10-17T22:37:07Z

@jorisvandenbossche ping when updated.

jorisvandenbossche · 2017-10-17T22:44:21Z

As far as I am concerned, this is updated. Is there a comment I overlooked?

jreback · 2017-10-17T22:47:32Z

pandas/core/dtypes/dtypes.py

@@ -326,6 +326,9 @@ def _validate_categories(categories, fastpath=False):
            if not categories.is_unique:
                raise ValueError('Categorical categories must be unique')

+        if isinstance(categories, ABCCategoricalIndex):


here you an also accept ABCCategorical

a Categorical has already been converted to CategoricalIndex

jorisvandenbossche added 3 commits October 16, 2017 10:15

BUG: Categorical(Index) passed as categories

93bb429

better check for categoricalIndex

7bc64bd

fix pep8

938e4e8

jreback requested changes Oct 16, 2017

View reviewed changes

TomAugspurger added this to the 0.21.0 milestone Oct 16, 2017

Merge remote-tracking branch 'upstream/master' into cat-constructor

ab23d41

add whatsnew

7fa8578

jschendel reviewed Oct 16, 2017

View reviewed changes

jreback added the Categorical Categorical Data Type label Oct 16, 2017

jreback reviewed Oct 16, 2017

View reviewed changes

fixup feedback

b020498

jorisvandenbossche added 2 commits October 16, 2017 20:47

move check to revert allowing non-unique Categoricals

ba532ee

typo in whatsnew

1c56abe

jreback reviewed Oct 17, 2017

View reviewed changes

jreback approved these changes Oct 17, 2017

View reviewed changes

jreback merged commit a0a0f5a into pandas-dev:master Oct 18, 2017

jorisvandenbossche deleted the cat-constructor branch October 18, 2017 07:53

yeemey pushed a commit to yeemey/pandas that referenced this pull request Oct 20, 2017

BUG: Categorical(Index) passed as categories (pandas-dev#17888)

e6fc4cd

alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

BUG: Categorical(Index) passed as categories (pandas-dev#17888)

1cb8b25

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

BUG: Categorical(Index) passed as categories (pandas-dev#17888)

5ed076c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Categorical(Index) passed as categories #17888

BUG: Categorical(Index) passed as categories #17888

jorisvandenbossche commented Oct 16, 2017 •

edited

Loading

jreback Oct 16, 2017

jorisvandenbossche Oct 16, 2017

jorisvandenbossche Oct 16, 2017

jreback Oct 16, 2017

jorisvandenbossche Oct 16, 2017

TomAugspurger commented Oct 16, 2017

codecov bot commented Oct 16, 2017

codecov bot commented Oct 16, 2017 •

edited

Loading

jschendel Oct 16, 2017

jorisvandenbossche Oct 16, 2017

jschendel Oct 16, 2017

jreback Oct 16, 2017 •

edited

Loading

jorisvandenbossche Oct 16, 2017

jorisvandenbossche commented Oct 16, 2017

jreback commented Oct 17, 2017

jorisvandenbossche commented Oct 17, 2017

jreback Oct 17, 2017

jorisvandenbossche Oct 17, 2017

jreback Oct 17, 2017

BUG: Categorical(Index) passed as categories #17888

BUG: Categorical(Index) passed as categories #17888

Conversation

jorisvandenbossche commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 16, 2017

codecov bot commented Oct 16, 2017

Codecov Report

codecov bot commented Oct 16, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 16, 2017

jreback commented Oct 17, 2017

jorisvandenbossche commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 16, 2017 •

edited

Loading

codecov bot commented Oct 16, 2017 •

edited

Loading

jreback Oct 16, 2017 •

edited

Loading